Review fixes#32
Merged
ScriptSmith merged 172 commits intomainfrom Apr 28, 2026
Merged
Conversation
Contributor
Greptile SummaryThis PR addresses the two previously flagged security issues (OIDC JWKS DNS rebinding, bootstrap auth shared "unknown" IP lockout bucket) and adds a broad set of related hardening: per-fetch DNS pinning for JWKS and discovery URLs, reserved-role stripping from IdP claims and proxy headers, tenant-scoped cache keys for response and semantic caches, bootstrap brute-force rate limiting, and proxy auth failing closed when Confidence Score: 5/5Safe to merge; both previously flagged P1/P0 issues are resolved and only P2 observations remain. All findings are P2 (minor information leak and a partial DNS-pinning gap for non-JWKS OIDC endpoints). No new P0/P1 issues introduced. The breaking proxy-auth change is guarded by a startup validation error. Test coverage for the new paths is thorough. src/auth/oidc.rs — token/userinfo endpoint calls still use unpinned http_client (P2). src/middleware/layers/admin.rs — bootstrap key length oracle via length-based early return (P2, documented trade-off). Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant AdminMiddleware
participant OidcAuthenticator
participant GatewayJwtRegistry
participant JwtValidator
participant Cache
participant DB
Client->>AdminMiddleware: Request with Bearer token
AdminMiddleware->>AdminMiddleware: strip_reserved_roles(claims.roles)
AdminMiddleware->>GatewayJwtRegistry: find_or_load_by_issuer(issuer, rate_limit)
GatewayJwtRegistry->>Cache: check_and_incr_rate_limit(gw:jwt:lazy_load:{ip})
Cache-->>GatewayJwtRegistry: allowed/denied
GatewayJwtRegistry->>DB: find_enabled_oidc_by_issuer
DB-->>GatewayJwtRegistry: SSO configs
GatewayJwtRegistry->>JwtValidator: with_options(config, UrlValidationOptions)
JwtValidator->>JwtValidator: refresh_jwks()
JwtValidator->>JwtValidator: validate_base_url_opts(jwks_url)
JwtValidator->>JwtValidator: pinned_reqwest_client(validated)
JwtValidator-->>AdminMiddleware: validated claims
Client->>OidcAuthenticator: OIDC callback
OidcAuthenticator->>OidcAuthenticator: validate_base_url_opts(discovery_url)
OidcAuthenticator->>OidcAuthenticator: pinned_reqwest_client - fetch discovery doc
OidcAuthenticator->>OidcAuthenticator: check issuer == config.issuer
OidcAuthenticator->>OidcAuthenticator: validate_base_url_opts(token/jwks/userinfo)
OidcAuthenticator->>OidcAuthenticator: strip_reserved_roles(claims.roles/groups)
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/auth/oidc.rs
Line: 553-573
Comment:
**Token/userinfo endpoints validated but not pinned on use**
SSRF validation for `authorization_endpoint`, `token_endpoint`, and `userinfo_endpoint` runs at discovery time, but subsequent calls to those endpoints (token exchange, userinfo fetch) still use the unpinned `self.http_client`. The `jwks_uri` case is fully fixed by the new per-fetch pinned client in `JwtValidator::refresh_jwks`, but a short-TTL DNS rebind between discovery and the token-exchange POST could redirect an auth-code delivery to an internal address. The impact is narrower than the JWKS case (the received tokens would still fail JWKS validation), but the gateway would be making an outbound POST to an attacker-controlled internal target.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/middleware/layers/admin.rs
Line: 745-748
Comment:
**Bootstrap key length exposed via length-based early return**
`could_be_bootstrap_guess` only returns `Ok(None)` (without touching the rate-limit counter) for keys whose length differs from the bootstrap key. An attacker can probe all key lengths: requests with wrong-length keys produce no lockout, while the matching length eventually triggers it. This makes the bootstrap key length trivially enumerable. The code comment acknowledges the trade-off (normal JWT bearer tokens must not exhaust the rate limit), but the information leak is worth noting for operators: bootstrap key length becomes observable through lockout behavior.
How can I resolve this? If you propose a fix, please make it concise.Reviews (7): Last reviewed commit: "Fix WebSocket race condition" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Owner
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.